Skip to content

Check isnan in maximum / minimum with CPU backend#2652

Merged
awni merged 3 commits intoml-explore:mainfrom
aisk:max-isnan
Nov 3, 2025
Merged

Check isnan in maximum / minimum with CPU backend#2652
awni merged 3 commits intoml-explore:mainfrom
aisk:max-isnan

Conversation

@aisk
Copy link
Copy Markdown
Contributor

@aisk aisk commented Oct 5, 2025

Proposed changes

Please include a description of the problem or feature this PR is addressing. If there is a corresponding issue, include the issue #.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@awni
Copy link
Copy Markdown
Member

awni commented Oct 6, 2025

Could you add a test for this so we know it's working as expected?

@aisk
Copy link
Copy Markdown
Contributor Author

aisk commented Oct 13, 2025

@aisk tests added.

Comment on lines +1065 to +1070
x = array({1.0f, NAN, 3.0f});
auto y = array({NAN, 2.0f, 1.0f});
auto max_result = maximum(x, y);
auto min_result = minimum(x, y);
CHECK(array_equal(max_result, array({NAN, NAN, 3.0f}), true).item<bool>());
CHECK(array_equal(min_result, array({NAN, NAN, 1.0f}), true).item<bool>());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these tests will hit the simd implementation because it will only be used for 8 or more elements. So could you add a tests with larger vectors, say 8 or 16?

Copy link
Copy Markdown
Member

@awni awni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@awni awni merged commit 1ff2b71 into ml-explore:main Nov 3, 2025
7 checks passed
@aisk aisk deleted the max-isnan branch March 3, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants